Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(number): add romanNumeral method #3070

Merged
merged 18 commits into from
Oct 31, 2024

Conversation

AmaanRS
Copy link
Contributor

@AmaanRS AmaanRS commented Aug 22, 2024

Adds #2649


Adds a method for generating random romanNumerals:

faker.number.romanNumeral() // "CMXCIII"
faker.number.romanNumeral(5) // "III"
faker.number.romanNumeral({ min: 10 }) // "XCIX"
faker.number.romanNumeral({ max: 20 }) // "XVII"
faker.number.romanNumeral({ min: 5, max: 10 }) // "VII"

Documentation

@AmaanRS AmaanRS requested a review from a team as a code owner August 22, 2024 06:43
Copy link

netlify bot commented Aug 22, 2024

Deploy Preview for fakerjs ready!

Name Link
🔨 Latest commit 6e04bef
🔍 Latest deploy log https://app.netlify.com/sites/fakerjs/deploys/6723548764693d0008135ec3
😎 Deploy Preview https://deploy-preview-3070.fakerjs.dev
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Shinigami92 Shinigami92 marked this pull request as draft August 22, 2024 15:00
@import-brain import-brain added c: feature Request for new feature p: 1-normal Nothing urgent m: number Something is referring to the number module labels Aug 24, 2024
@import-brain import-brain added this to the v9.x milestone Aug 24, 2024
Copy link
Member

@import-brain import-brain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add tests for romanNumeral

src/modules/number/index.ts Outdated Show resolved Hide resolved
src/modules/number/index.ts Outdated Show resolved Hide resolved
src/modules/number/index.ts Outdated Show resolved Hide resolved
src/modules/number/index.ts Outdated Show resolved Hide resolved
src/modules/number/index.ts Outdated Show resolved Hide resolved
src/modules/number/index.ts Outdated Show resolved Hide resolved
@AmaanRS
Copy link
Contributor Author

AmaanRS commented Aug 28, 2024

In this commit 7abd2c7 i created a new function intToRomanNumeral so where should i place it ?

src/modules/number/index.ts Outdated Show resolved Hide resolved
src/modules/number/index.ts Outdated Show resolved Hide resolved
src/modules/number/index.ts Outdated Show resolved Hide resolved
src/modules/number/index.ts Outdated Show resolved Hide resolved
@AmaanRS
Copy link
Contributor Author

AmaanRS commented Aug 29, 2024

How should i write random seeded test for "it('should generate a Roman numeral between 1 and 10')" ? Since i don't what answer will be.

@AmaanRS
Copy link
Contributor Author

AmaanRS commented Aug 29, 2024

And there will be no value range test ?

@matthewmayer
Copy link
Contributor

There's a few ways you could do it. For 1 to 10 you could check if it's any of the values from I to X by listing all 10 values.

You could also check it only contains suitable characters.

Also add tests that check hitting each of the errors and check appropriate error is thrown.

@AmaanRS
Copy link
Contributor Author

AmaanRS commented Aug 30, 2024

Please let me know if there are any changes with the code and test if not i will run preflight and then commit the code

@matthewmayer
Copy link
Contributor

Ideally please run preflight after every change. This will help catch any minor syntax errors etc so the reviewers can focus on the logic of your code rather than nitpicking.

src/modules/number/index.ts Show resolved Hide resolved
src/modules/number/index.ts Outdated Show resolved Hide resolved
src/modules/number/index.ts Outdated Show resolved Hide resolved
src/modules/number/index.ts Outdated Show resolved Hide resolved
test/modules/number.spec.ts Outdated Show resolved Hide resolved
src/modules/number/index.ts Outdated Show resolved Hide resolved
src/modules/number/index.ts Outdated Show resolved Hide resolved
@AmaanRS
Copy link
Contributor Author

AmaanRS commented Sep 1, 2024

I ran into an error during preflight FakerApiDocsProcessingError: Failed to process parameter options at src/modules/number/index.ts:468:16 : Expected exactly one element for jsdocs, got 0

@matthewmayer
Copy link
Contributor

Expected exactly one element for jsdocs

its not a very intuitive error message but i think the problem is that you're missing a @since JSDoc tag

add

@since 9.1.0 at the bottom of the JSDoc as this likely wont be in the 9.0.0 release.

@ST-DDT
Copy link
Member

ST-DDT commented Sep 1, 2024

I ran into an error during preflight FakerApiDocsProcessingError: Failed to process parameter options at src/modules/number/index.ts:468:16 : Expected exactly one element for jsdocs, got 0

The error is not caused by @since. It is caused by the options.min/max parameter not having jsdocs.
I'll improve the error message.

FakerApiDocsProcessingError: Failed to process property 'options.min' at src/modules/number/index.ts:467:36 : Expected exactly one element for jsdocs, got 0

The options object should be defined like this:

  romanNumeral(
    options:
      | number
      | {
          /**
           * Lower bound for generated number.
           *
           * @default 1
           */
          min?: number;
          /**
           * Upper bound for generated number.
           *
           * @default 3999
           */
          max?: number;
        } = {}
  ): string {

src/modules/number/index.ts Outdated Show resolved Hide resolved
src/modules/number/index.ts Outdated Show resolved Hide resolved
src/modules/number/index.ts Outdated Show resolved Hide resolved
test/modules/number.spec.ts Outdated Show resolved Hide resolved
@ST-DDT
Copy link
Member

ST-DDT commented Oct 12, 2024

@AmaanRS Could you please address the requested changes?

@ST-DDT ST-DDT linked an issue Oct 17, 2024 that may be closed by this pull request
Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last change, then this is good to go.

test/modules/number.spec.ts Outdated Show resolved Hide resolved
@ST-DDT
Copy link
Member

ST-DDT commented Oct 19, 2024

Please also change the description of the PR to something explaining the feature in more detail. A few sentences are sufficient.

ST-DDT
ST-DDT previously approved these changes Oct 20, 2024
@ST-DDT ST-DDT requested review from matthewmayer and a team October 20, 2024 15:35
src/modules/number/index.ts Outdated Show resolved Hide resolved
@ST-DDT ST-DDT requested a review from a team October 29, 2024 12:25
@ST-DDT
Copy link
Member

ST-DDT commented Oct 29, 2024

@Shinigami92 and @import-brain Could you please re-review this PR (or dismiss your stale reviews)?

@Shinigami92 Shinigami92 dismissed their stale review October 29, 2024 12:29

PR has been updated

@import-brain import-brain dismissed their stale review October 29, 2024 13:02

Review is stale

@ST-DDT ST-DDT enabled auto-merge (squash) October 31, 2024 09:57
@ST-DDT ST-DDT merged commit 72937de into faker-js:next Oct 31, 2024
21 checks passed
@ST-DDT
Copy link
Member

ST-DDT commented Oct 31, 2024

@AmaanRS Thanks for your first contribution. ❤️
I hope you had a good experience contributing.
If you have any suggestions on how to improve things, please let us know.

This feature will be released in Faker v9.2, which will probably release shortly (a week to a few weeks).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: feature Request for new feature m: number Something is referring to the number module p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for generating Roman Numerals
5 participants